Feature: create/delete instruments #513

yinanazhou · 2026-02-06T20:11:47Z

init create/delete instrument feature
refactor add name frontend to name row manager

preview:

- Add CreateInstrument Django view with form handling and validation - Implement frontend UI with CreateInstrument TypeScript module - Add CreateInstrumentValidator for frontend form validation - Add CreateInstrumentManager for API interactions and DatabaseService - Create error_codes and exceptions modules for structured error handling - Add migration 0012 for instrument creation feature - Update InstrumentName and AVResource models to support creation workflow - Update management commands (import, download, index) for creation compatibility - Update instrument views and templates with creation UI - Update Docker Compose and nginx configurations - Add comprehensive test suite for creation workflows ref: #406

- Add Django DELETE endpoint with permission checks for user-created instruments - Add confirmation modal template and delete button on detail page - Add TypeScript DeleteInstrumentManager for modal interaction and API call - Add Solr cleanup on successful deletion via transaction.on_commit - Restrict delete access to instrument creator and superusers

- Create NameRowManager class to extract duplicated name row management logic (row creation, RTL support, add/remove rows, form data collection) - Refactor AddNameManager to use NameRowManager via composition - Refactor CreateInstrumentManager to use NameRowManager via composition - Move AddName publish logic from entry point into AddNameManager for consistency - Fix DeleteNameManager to use shared getCsrfToken utility instead of private method - Update entry points (AddName.ts, CreateInstrument.ts) to follow consistent initialization pattern

refs: #505 (comment)

refs: #510

PouyaMohseni

Apart from that, I also cannot access the instrument list endpoint. I have dropped all public schemas in Django and flushed the database, yet I still receive the following error when accessing the /instrument endpoint:

NoReverseMatch
django.urls.exceptions.NoReverseMatch: Reverse for 'instrument-detail' with arguments '('',)' not found. 1 pattern(s) tried: ['instrument/(?P<umil_id>[-a-zA-Z0-9_]+)/\\Z']

Do you have any suggestions for reindexing?

PouyaMohseni · 2026-02-09T14:41:06Z

web-app/django/VIM/apps/instruments/models/instrument.py

+        related_name="created_instruments",
+        help_text="User who created this instrument (null for Wikidata imports)",
+    )
+    source = models.CharField(


Did we decide to require sources for newly added entries? I believe Ich mentioned that an instrument's source should be defined by the sources of its names.

I'm not sure which approach is the best, but at the moment we don't seem to have a clear editorial workflow for instrument names. For example, what should happen if a verified instrument ends up with no verified names (e.g., all names are removed)? Should the instrument entry itself be deleted, or should it remain as an instrument without any names?

If we decide not to require sources for instruments, the solution is straightforward delete. However, if instruments do have sources, the action becomes less clear.

See #406 (comment)

PouyaMohseni · 2026-02-09T14:56:59Z

web-app/django/VIM/apps/instruments/utils/validators.py

+    if not hbs_class:
+        return False
+    # Pattern: one digit (1-5), followed by another digit, optionally followed by more .digits
+    pattern = r"^[1-5][0-9](\.[0-9]+)*$"


I don't think this is a correct validator for HBS numbers. HBS values can include plus and dash in addition to a dot. Also, the second number, like the first, should be between 1 and 5. More, 0 is not a valid value in HBS numbers. I verified this by checking the entries we pulled from Wikidata. You can also see the same function referenced in #511 NameValidator.ts.

This is why I suggested you to put #511 on hold. Again, this is the MVP for the instrument creation feature. The main idea is to make it work. All the addition touchup should be addressed in separate PRs.

PouyaMohseni · 2026-02-09T15:27:42Z

web-app/django/VIM/templates/instruments/detail.html

-                <a class="view-field notranslate"
-                   href="https://www.wikidata.org/wiki/{{ instrument.wikidata_id }}"
-                   target="_blank">{{ instrument.wikidata_id }}</a>
+                {% if instrument.is_user_created %}


I think the assumption that a user created instrument does not have a Wikidata ID is wrong. When pushed to Wikidata, they are indexed. So maybe change it to whether wikidata_id exists, as what you did with mimo_class.

PouyaMohseni · 2026-02-09T15:36:35Z

web-app/django/VIM/templates/instruments/includes/addName.html

          <span>Add New Name: <span id="instrumentNameInModal" class="notranslate m-1"></span></span>
          <br />
-          <small>Wikidata ID: <span id="instrumentWikidataIdInModal" class="notranslate m-1"></span></small>
+          <small>UMIL ID: <span id="instrumentUmilIdInModal" class="notranslate m-1"></span></small>


Do we need UMIL ID here as it is something internal? Also, we don't show that in instrument detail. Mybe showing that only when Wikidata ID is null?

PouyaMohseni · 2026-02-09T15:57:22Z

web-app/django/VIM/templates/instruments/detail.html

+                           class="rounded mw-50"
+                           onerror="this.onerror=null;this.src='{% static "assets/images/instruments/no-image.svg" %}';" />
+                      <div class="ms-2 align-self-start">
+                        {% if instrument.is_user_created and instrument.default_image.source_name %}


I don't think showing any uploaded image without approval to logged-out users would be a good idea, as we are not sure about the license for use and the content.

This should be a separate issue/PR

PouyaMohseni · 2026-02-09T15:57:41Z

web-app/django/VIM/templates/instruments/detail.html

+                           class="rounded mw-50"
+                           onerror="this.onerror=null;this.src='{% static "assets/images/instruments/no-image.svg" %}';" />
+                      <div class="ms-2 align-self-start">
+                        {% if instrument.is_user_created and instrument.default_image.source_name %}


Maybe showing all image sources?

PouyaMohseni · 2026-02-09T16:04:21Z

web-app/django/VIM/apps/instruments/views/instrument_detail.py

+    slug_field = "umil_id"
+    slug_url_kwarg = "umil_id"
+
+    def get_object(self, queryset=None):


I am curious whether there is a good approach to have UMIL- before the instrument ID in every request.

wikidata_id is no longer required with the changes, using UMIL- is a better approach than exposing pk in the database

PouyaMohseni · 2026-02-09T16:11:26Z

web-app/django/VIM/apps/instruments/views/delete_instrument.py

+            instrument.avresource_set.all().delete()
+
+            # Delete the instrument (InstrumentNames cascade automatically)
+            instrument.delete()


I was wondering whether it is better to perform soft delete or to retire the current UMIL id.

It is, but it should be handled in a separate issue/PR

PouyaMohseni · 2026-02-09T16:15:38Z

web-app/django/VIM/apps/instruments/views/delete_instrument.py

+            )
+
+        # Only the creator or a superuser can delete
+        if not (request.user.is_superuser or instrument.created_by == request.user):


Should the original contributor of an instrument always have the ability to delete the instrument? There might be many other name contributions to that entry by other users, or we might have already pushed the instrument entry to Wikidata. If a user decides to delete their contribution, it could undermine the other contributions.

Again, this should be handled in a followup issue/PR

PouyaMohseni

Apart from that, I also cannot access the instrument list endpoint. I have dropped all public schemas in Django and flushed the database, yet I still receive the following error when accessing the /instrument endpoint:

NoReverseMatch
django.urls.exceptions.NoReverseMatch: Reverse for 'instrument-detail' with arguments '('',)' not found. 1 pattern(s) tried: ['instrument/(?P<umil_id>[-a-zA-Z0-9_]+)/\\Z']

Do you have any suggestions for reindexing?

yinanazhou · 2026-02-09T18:29:12Z

Do you have any suggestions for reindexing?

Have you migrated yet?

yinanazhou added 8 commits January 26, 2026 15:34

chore: update LLM files in .gitignore

b0aca63

chore: update vite config

6110c55

test: update instrument url to use umil_id

eb871a5

ci: update pre commit version & config

ee6e146

test: CRUD test for name and instrument

f2c14fa

yinanazhou requested a review from PouyaMohseni February 6, 2026 20:11

yinanazhou added 6 commits February 6, 2026 15:19

fix: add html direction null check

f8656f3

refs: #505 (comment)

chore: remove duplicate entries in .gitignore

6a337e9

refs: #505 (comment)

chore: update docker-compose to docker compose

a80cdba

refs: #505 (comment)

fix: correct syntax for error message

086d662

refs: #505 (comment)

chore: add comment for Wikidata languageinfo

48d06ae

refs: #505 (comment)

fix: index add/delete names

0b46f56

refs: #510

PouyaMohseni requested changes Feb 9, 2026

View reviewed changes

PouyaMohseni reviewed Feb 9, 2026

View reviewed changes

PouyaMohseni approved these changes Feb 10, 2026

View reviewed changes

yinanazhou merged commit b6815a2 into develop Feb 10, 2026
8 checks passed

Feature: create/delete instruments #513

Feature: create/delete instruments #513

Uh oh!

Conversation

yinanazhou commented Feb 6, 2026

Uh oh!

PouyaMohseni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yinanazhou Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PouyaMohseni left a comment

Choose a reason for hiding this comment

Uh oh!

yinanazhou commented Feb 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yinanazhou Feb 9, 2026 •

edited

Loading